[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled#4333
[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled#4333mike-dubrovsky wants to merge 1 commit into
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Hi, thank you for the work on adding FIB‑suppression support across orchagent and fpmsyncd.
This is required for the RouteOrch tests to build and appears to be another file that the PR may have unintentionally missed? After defining gEnableFibSuppress, the following failing RouteOrch tests all passed:
Question: Did the upstream CI pipeline succeed or did I miss something from the PR? |
3120ae9 to
6a7694d
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@mike-dubrovsky , please follow the description template "why i did", "how tested" etc. Also update results from sonic-mgmt tests. @dgsudharsan for viz. |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce route programming overhead by avoiding APPL_STATE_DB route-state writes/notifications when BGP FIB suppression is disabled, and by making FIB suppression a startup-time decision coordinated between orchagent and fpmsyncd.
Changes:
- Add an
orchagentstartup flag (-F) to enable FIB suppression and guardRouteOrch::publishRouteState()behind it. - When suppression is disabled, clear stale route entries from
APPL_STATE_DBduringRouteOrchconstruction. - Update
fpmsyncdto read suppression enablement at startup (no longer dynamically via CONFIG_DB subscription), and adjust VS/mock tests accordingly.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
orchagent/main.cpp |
Adds -F CLI parsing and global flag to control suppression behavior. |
orchagent/routeorch.cpp |
Skips publishing route state when suppression is off; clears stale APPL_STATE_DB route entries when suppression is off. |
fpmsyncd/fpmsyncd.cpp |
Removes CONFIG_DB subscription logic and reads suppression state at startup from STATE_DB. |
tests/test_route.py |
Restarts swss/fpmsyncd in the route offload test so suppression state changes take effect. |
tests/mock_tests/routeorch_ut.cpp |
Forces suppression enabled for publish-related unit tests. |
You can also share your feedback on Copilot code review. Take the survey.
|
@vpandian-nokia, I’m sorry — I initially posted an incomplete diff. I think this version is better and tested. I will address review comments in a day or two. |
6a7694d to
a4f788f
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
a4f788f to
2f62db4
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@dgsudharsan Could you please help me with a code review? |
|
@deepak-singhal0408 thanks for your patience I've updated |
7ce7866 to
5498255
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
5498255 to
e502f66
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
e502f66 to
11a21f0
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
sonic-net#26175) (sonic-net#26176) ### Description of PR Summary: Fixes sonic-net#26175 Temporarily skip the whole `bgp/test_bgp_suppress_fib.py` module for the suppress-fib-pending **dynamic-toggle race**. The module toggles `suppress-fib-pending` dynamically at runtime (via the shared `config_bgp_suppress_fib()` helper, no `config reload`). That triggers a race between **orchagent, fpmsyncd and FRR** where routes are intermittently **not marked offloaded in FRR** / **not propagated to the upstream VM** within the wait window, failing essentially every case in the module. The root-cause fix makes `suppress-fib-pending` **static** (takes effect only after a `config reload`), removing the race — [sonic-swss#4333](sonic-net/sonic-swss#4333), [sonic-buildimage#26151](sonic-net/sonic-buildimage#26151), [sonic-utilities#4361](sonic-net/sonic-utilities#4361), [sonic-mgmt#22916](sonic-net#22916), [SONiC#2335](sonic-net/SONiC#2335). > **The product fix is master-only and is not backported.** Because of that, the skip uses **two different lift semantics**, expressed as separate conditions: > > - **`master`** — gated on the tracking issue: `release in ['master'] and https://github.com/sonic-net/sonic-mgmt/issues/26175`. When the fix lands on master and sonic-net#26175 is closed, the skip **auto-lifts** on master and coverage returns — no manual edit needed. > - **`202505` / `202511` / `202605`** — a plain `release in [...]` condition with **no issue gate**. These branches never receive the product fix, so the module is skipped for the life of the branch (same nature as the existing pre-202411 "not supported" condition). > > **Per-branch effect still requires cherry-picking this skip.** Each release branch's nightly runs its own `sonic-mgmt` checkout, so this YAML change must be cherry-picked to 202505/202511/202605 for the skip to take effect there. The identical diff matches on each branch via its own `release` token. > > If a release branch ever does receive the product fix, drop that branch's token from the `release in [...]` list — a deliberate per-branch edit. sonic-net#26175 tracks the lifecycle. ### Type of change - [ ] Bug fix - [x] Testbed and Framework(new/improvement) - [ ] New Test case - [ ] Skipped for non-supported platforms - [ ] Test case improvement ### Back port request - [ ] 202205 - [ ] 202305 - [ ] 202311 - [ ] 202405 - [ ] 202411 - [x] 202505 - [x] 202511 (also 202605 — not listed in the template checkboxes; cherry-pick required there as well.) ### Approach #### What is the motivation for this PR? The `test_bgp_suppress_fib` module is a persistent nightly-noise source across master and the active release branches. The dynamic runtime toggle of `suppress-fib-pending` races orchagent/fpmsyncd/FRR, so routes are intermittently left un-offloaded / un-propagated and nearly every case in the module fails intermittently. Skipping the module de-noises the nightly signal while the product-layer fix rolls out on master. #### How did you do it? Extended the existing `bgp/test_bgp_suppress_fib.py` skip block with `conditions_logical_operator: or` and split the affected releases by lift semantics: ```yaml conditions: - "release in ['201811', '201911', '202012', '202205', '202211', '202305', '202311', '202405']" - "release in ['202505', '202511', '202605']" - "release in ['master'] and sonic-net#26175" ``` The `conditional_mark` plugin replaces the issue URL with `True`/`False` based on the issue's open/closed state and `eval()`s the condition, so the master line auto-lifts when sonic-net#26175 closes while the release-branch line stays permanent. (The same file already uses the `<expr> and <issue-url>` form for the `asic_type in ['vs'] and sonic-net#14449` conditions.) #### How did you verify/test it? - `tests_mark_conditions.yaml` parses cleanly and passes the `check_conditional_mark_sort.py` pre-commit hook. - Simulated the `conditional_mark` evaluation for each `release` value: - issue **open**: master + 202505/202511/202605 all skip; - issue **closed**: master unskips, 202505/202511/202605 remain skipped — matching the intended lift semantics. #### Any platform specific information? None — the skip is keyed on `release` only (all ASICs/topologies), matching the all-vendor nature of the race. #### Supported testbed topology if it's a new test case? N/A — not a new test case; module skip only. ### Documentation N/A Signed-off-by: Deepak Singhal <deepsinghal@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ssithaia-ebay <ssithaian@ebay.com>
|
@mike-dubrovsky, please rebase this PR. Need #4761 change to be picked up for Ci to pass. And also resolve addressed comments. |
11a21f0 to
a9240d3
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
CI: Both params fail at the precondition, before any suppression is asserted:
|
a9240d3 to
ea0b2fa
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
ea0b2fa to
d2ddd09
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Route download speed degraded after BGP prefix suppression was introduced. RouteOrch::publishRouteState() unconditionally writes to APPL_STATE_DB and sends a pub/sub notification on every route add/remove, even when FIB suppression is disabled and no consumer needs the data. Changes: Config changes take effect after a full config reload or device reboot, not dynamically at runtime. This eliminates multi-process race conditions between orchagent, fpmsyncd, and FRR. - orchagent: add -F command-line flag (gEnableFibSuppress) to control FIB suppression at startup; guard publishRouteState() to return early when suppression is disabled - fpmsyncd: read suppress-fib-pending from CONFIG_DB at startup; remove dynamic CONFIG_DB subscription (SubscriberStateTable) since config changes now require a config reload or device reboot - fpmsyncd: conditionally create routeResponseChannel and add it to the select loop only when suppression is enabled - tests: update mock tests to set gEnableFibSuppress before exercising publishRouteState; update integration test to restart swss/fpmsyncd after toggling suppress-fib-pending Signed-off-by: mike-dubrovsky <mdubrovs@cisco.com>
d2ddd09 to
bee72a5
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Why I did it
Route download speed degraded after BGP prefix suppression was introduced. RouteOrch::publishRouteState() unconditionally writes to APPL_STATE_DB and sends a pub/sub notification on every route add/remove, even when FIB suppression is disabled and no consumer needs the data.
What I did
Made publishRouteState() a no-op until FIB suppression is enabled.
Made "suppress-fib-pending" config changes take effect only after swss restart or config reload,
not dynamically at runtime. This eliminates multi-process race conditions between orchagent, fpmsyncd, and FRR.
Detailed description:
The CLI (config suppress-fib-pending) writes to DEVICE_METADATA and prints a message telling the user to perform config save and config reload for the change to take effect. This is in the sonic-utilities PR (#4361).
Detailed Changes in this PR:
How I verified it
UT and sonic-mgmt script.
Details if related
This change needs to go together with
sonic-net/sonic-buildimage#26151
sonic-net/sonic-utilities#4361
sonic-net/sonic-mgmt#22916
sonic-net/SONiC#2335